Add typings for EuiKeyPad #1229
Conversation
There was a problem hiding this comment.
I'm no TS expert here, so I'll let @chandlerprall handle the review.
But you do still need to add a changelog entry.
| } | ||
|
|
||
| export interface EuiKeyPadMenuItemProps extends EuiKeyPadMenuItemCommonProps { | ||
| href: string; |
There was a problem hiding this comment.
shouldn't need to define href here as it's covered by HTMLAttributes<HTMLAnchorElement> when defining EuiKeyPadMenuItem
There was a problem hiding this comment.
Yeah both of them are actually covered. My thought was, to make it more explicit that way, that they are taken by the component. I can remove both if you want.
There was a problem hiding this comment.
Let's remove them; I think there's more of a chance they'd be a distraction when looking at this file than an aid. They were for me at least: 🤔this extends a button and has an explicit onClick, what's different about it?
There was a problem hiding this comment.
BTW this will also imply that href is required and can't be null or undefined.
If we are going to say: ok you can add or not an href props to that component I think it's fine to have the HTMLAttributes<HTMLAnchorElement> type manage this. If you want to force the component to have a non null non undefined href I think this is one way to go.
Is href required?
There was a problem hiding this comment.
href prop is not marked isRequired. Neither is onClick on EuiKeyPadMenuItemButton
| } | ||
|
|
||
| export interface EuiKeyPadMenuItemButtonProps extends EuiKeyPadMenuItemCommonProps { | ||
| onClick: (ev: MouseEventHandler<HTMLButtonElement>) => void; |
There was a problem hiding this comment.
shouldn't need to define onClick here as it's covered by HTMLAttributes<HTMLButtonElement> when defining EuiKeyPadMenuItemButton
|
@timroes @chandlerprall I rewrite here my comment in case you missed it: |
|
They are optional, so I removed them. |
Summary
Adding typings for
EuiKeyPadand its item components.Checklist
[ ] This was checked in mobile[ ] This was checked in IE11[ ] This was checked in dark mode[ ] Any props added have proper autodocs[ ] Documentation examples were added[ ] A changelog entry exists and is marked appropriately[ ] This was checked for breaking changes and labeled appropriately[ ] Jest tests were updated or added to match the most common scenarios[ ] This was checked against keyboard-only and screenreader scenarios